feat: OAuth security integration for partner applications #763
Conversation
| "oauth_refresh_token_validity": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Description: "Specifies how long refresh tokens should be valid (in seconds). OAUTH_ISSUE_REFRESH_TOKENS must be set to TRUE.", |
There was a problem hiding this comment.
do we need oauth_issue_refresh_tokens or would the presence/absence of oauth_refresh_token_validity give us enough information?
There was a problem hiding this comment.
I see that keeping both would more closely match Snowflake's api, so we might want to keep as is
There was a problem hiding this comment.
Looks like oauth_issue_refresh_tokens is true by default, so you need it here if you want to set it to false. However, oauth_refresh_token_validity is optional too, it defaults to various values depending on the client (10 hours for Tableau Desktop, 90 days for Tableau Server, etc) - there will be cases where people omit both or specify only one.
Co-authored-by: Eduardo Lopez <[email protected]>
pkg/resources/oauth_integration.go
Outdated
| stmt.SetString(`OAUTH_USE_SECONDARY_ROLES`, d.Get("oauth_use_secondary_roles").(string)) | ||
| } | ||
| if _, ok := d.GetOk("blocked_roles_list"); ok { | ||
| stmt.SetStringList(`BLOCKED_ROLES_LIST`, expandStringList(d.Get("blocked_roles_list").([]interface{}))) |
There was a problem hiding this comment.
I think this'll now panic because blocked_roles_list is a set, not []interface{}
There was a problem hiding this comment.
I can't get acceptance tests to run locally, so hopefully this fixes it. Found a similar instance in network policies and copied it:
expandStringList(d.Get("blocked_roles_list").(*schema.Set).List())
|
/ok-to-test sha=d10af87 |
|
Integration tests failure for d10af87 |
Looks like there might be a panic here: https://github.com/chanzuckerberg/terraform-provider-snowflake/runs/4301715646?check_suite_focus=true#step:9:1050 (not sure if this log is hidden behind GitHub access controls) |
|
/ok-to-test sha=3eb42aa |
|
Integration tests failure for 3eb42aa |
|
CI step |
When you click on the link in this comment, what does it say? #763 (comment) |
Sorry, this works. I was previously clicking on 'integration-trusted' in the footer (force of habit). |
|
Found the problem! Silly copy-paste typo 🤦 |
|
/ok-to-test sha=ba75402 |
|
Integration tests failure for ba75402 |
|
Hmm, are these all meant to be treated as strings and parsed to booleans/integers? https://github.com/gouline/terraform-provider-snowflake/blob/161f112/pkg/resources/oauth_integration.go#L186-L201 case "OAUTH_ISSUE_REFRESH_TOKENS":
if err = d.Set("oauth_issue_refresh_tokens", v.(bool)); err != nil {
return errors.Wrap(err, "unable to set OAuth issue refresh tokens for security integration")
}
case "OAUTH_REFRESH_TOKEN_VALIDITY":
if err = d.Set("oauth_refresh_token_validity", v.(int64)); err != nil {
return errors.Wrap(err, "unable to set OAuth refresh token validity for security integration")
}
case "OAUTH_USE_SECONDARY_ROLES":
if err = d.Set("oauth_use_secondary_roles", v.(string)); err != nil {
return errors.Wrap(err, "unable to set OAuth use secondary roles for security integration")
}
case "BLOCKED_ROLES_LIST":
if err = d.Set("blocked_roles_list", strings.Split(v.(string), ",")); err != nil {
return errors.Wrap(err, "unable to set blocked roles list for security integration")
}I assumed from the descRows := sqlmock.NewRows([]string{
"property", "property_type", "property_value", "property_default",
}).AddRow("ENABLED", "Boolean", true, false)Latest commit treats them as strings, so hopefully that works 🤷 |
|
/ok-to-test sha=c165355 |
|
Integration tests failure for c165355 |
|
Previous error looks fixed now, latest commit should fix the new error. |
|
/ok-to-test sha=e09bf36 |
|
Integration tests failure for e09bf36 |
|
Should be fixed again. |
|
/ok-to-test sha=c2634c8 |
|
Integration tests failure for c2634c8 |
|
This one was interesting: |
|
/ok-to-test sha=e40332d |
|
Integration tests success for e40332d |
Implementing OAuth security integration, largely based on the existing SCIM security integration that's quite similar.
This one only covers the partner applications (i.e. Tableau Desktop, Tableau Server and Looker), however extending it to support custom OAuth clients would just involve adding required and optional parameters.
Test Plan
References